fix: only treat a reachability confirmation as a recovery when the app was offline#96504
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
… the effective URL Extract the repeated prev-reset/token/refresh sequence into armReachabilityRecovery(), and compare the effective reachability URL instead of mirroring the raw staging toggle — the raw value can change without changing the URL (and vice versa on staging, where null falls back to the environment default). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 588423ac34
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@mkhutornyi Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.movweb2.mov |
|
Suggested QA Steps: Setup (how to observe reconnects): Open the app in Chrome, open DevTools → Network tab, and type 1. Healthy boot — no phantom reconnects
2. Force Offline toggle → exactly one reconnect
3. Real connectivity loss → one reconnect on return
4. Rapid offline/online cycling doesn't storm
5. Staging-server toggle doesn't trigger a storm (staging/dev builds only)
In every test:
|
|
I can't run change requests for your access level. I can investigate, or file an issue in Failed after 0s · 0 tools used · view debug log |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 954a4140c0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
@mkhutornyi this happened to you while testing?
|
No. It's a pass ✅ |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.43-0 🚀
|
|
🤖 Help site review — no changes required I reviewed the changes in this PR and confirmed that no updates to Expensify's help site files under Why: This PR is a purely internal networking fix. It changes:
None of this alters any user-facing feature, setting, tab, button, or workflow. It's an under-the-hood change to how the app detects genuine offline→online transitions versus fake recovery signals (boot emissions, @adhorodyski — since no help site changes are needed, there's no linked help site PR to review. If you believe this change does affect documented behavior, reply with details and I'll draft the docs update. |
Explanation of Change
Production traces show serial Ping+ReconnectApp storms (up to 23 ReconnectApp calls in 52 seconds in one session) where nothing about the network changed. Root cause:
NetworkStatefiresreconnectApp()from raw NetInfonull→truetransitions, and those get manufactured with no real connectivity change by:undefined→null→trueslipping past the dead!== undefinedguard.NetInfo.refresh()re-emitting cached state after the debug paths resetprevIsInternetReachable = null.SHOULD_USE_STAGING_SERVEROnyx callback rebuilding the NetInfo subscription on every write, where each rebuild fires its own extra Ping.Fix:
getIsOffline()is true at the moment reachability is confirmed, or when a one-shotpendingReachabilityRecoverytoken was armed. The three debug paths (setForceOffline(false),setFailAllRequests(false),simulatePoorConnection(false)) arm the token instead of exploiting the transition loophole, so debug recovery stays Ping-verified.suppressNextReachabilityRestored, the old post-reconfigure band-aid. The gate above covers it.SHOULD_USE_STAGING_SERVERcallback rebuilds the subscription only when the effective reachability URL changed. Same-value rewrites, and toggle writes that do not change the URL (like on production, where the staging flag is forced off), no longer tear down NetInfo.Tests: one existing test (
null→true fires reconnect listener) asserted the buggy behavior and was inverted. New regression tests pin that a barenull→trueand repeated fake pairs never fire a reconnect, thatfalse→trueandfalse→null→truefire exactly once, that force-offline debug recovery still works, and that staging toggle writes which do not change the URL do not reconfigure. All affected suites pass, includingSequentialQueueTestandAPITest.Fixed Issues
$ #96634
PROPOSAL:
Tests
Offline tests
Same as steps 2 and 3 above. Both exercise the offline→online recovery path.
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari